-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Example usage #4
Conversation
…e that can also be given in the GitHub wiki, WIP;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks good, but please answer the questions.
src/test/java/de/unijena/cheminf/fragment/fingerprint/ExampleUsageTest.java
Outdated
Show resolved
Hide resolved
while (tmpSDFReader.hasNext()) { | ||
IAtomContainer tmpMolecule = tmpSDFReader.next(); | ||
tmpFragmenter.generateFragments(tmpMolecule); | ||
IAtomContainer[] tmpFragments = tmpFragmenter.getFragmentsAsContainers(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the usage of getFragmentsAsContainers() and not getFragments(), which returns them already as unique SMILES?
At the end, there is no big difference. I ask out of interest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like to have full control via my own SmilesGenerator instance here. And I think it is also important in this context to show this explicitly. Not every fragmentation functionality will have this kind of convenience function. In addition, every other string-based representation could be used instead of SMILES.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a brief comment will make this even clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment in last commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the example very illustrative and a very appropriate chemical example.
and i can gladly add real tests
src/test/java/de/unijena/cheminf/fragment/fingerprint/ExampleUsageTest.java
Show resolved
Hide resolved
That is wonderful to hear! If you think all is fine, please approve and merge the pull request. |
Wonderful |
Hi @B-s123 ,
I have created a new test class with two methods illustrating the basic usage of the fragment fingerprinter functionality. My suggestion would be to turn those also into the basic usage given in the GitHub wiki. Could you have a look at it and tell me what you think?
Kind regards,
Jonas